Restore DeepSeek-V4 tool calling for AgentX evals#2295
Conversation
cquil11
added
agentx
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
|
Claude finished @cquil11's task in 4m 9s —— View job Review
SummaryFound 1 blocking issue (inline comment on Everything else checks out:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| description: | ||
| - "Eval-only follow-up to PR #2231: restore the official DeepSeek-V4 vLLM tool-calling frontend flags --tool-call-parser deepseek_v4 and --enable-auto-tool-choice" | ||
| - "Re-run the dedicated AgentX SWE-bench eval to verify tool definitions are accepted instead of failing with 'tool parsing is disabled by frontend configuration'" | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2231 |
There was a problem hiding this comment.
🔴 BLOCKING: Appended entry's pr-link points to #2231, but this PR is #2295.
Why it matters: The PR CI gate passes (it runs validate_perf_changelog.py without a PR number), but the merge path does not. utils/prepare_perf_changelog_merge.py (invoked via utils/merge_with_reuse.sh) calls compare_entries(..., pr_number) → validate_added_pr_link(), which requires every entry appended by a PR to use that PR's own link or an XXX placeholder (utils/validate_perf_changelog.py:144-160). It will raise ChangelogValidationError: new PR entry must use 'https://github.com/SemiAnalysisAI/InferenceX/pull/2295' or an XXX placeholder; found '.../pull/2231' and block the merge. utils/recover_failed_ingest.py:326 likewise matches the ingest entry against the merging PR's link.
Fix: Point pr-link at this PR — the description lines already reference #2231, so no context is lost:
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2231 | |
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2295 |
There was a problem hiding this comment.
The pr-link bug (inline comment) is a real, verified issue — it will pass CI but abort utils/merge_with_reuse.sh at merge time, so this needs a fix before merging. Deferring to human review.
Extended reasoning...
Overview
Small, narrowly scoped follow-up to #2231: adds two vLLM server flags (--tool-call-parser deepseek_v4, --enable-auto-tool-choice) to a single benchmark launch script, plus one appended perf-changelog entry marked evals-only: true. No production/serving code, no auth/crypto/permissions surface.
Security risks
None. This only touches a benchmark shell script (adds two well-known vLLM CLI flags) and a changelog YAML entry. No user input handling, no secrets, no injection surface introduced.
Level of scrutiny
Low-risk in terms of blast radius (benchmark-only, eval-labeled), but I independently verified the inline pr-link finding by reading `utils/validate_perf_changelog.py` and `utils/merge_with_reuse.sh`: the appended entry's `pr-link` points at PR #2231 instead of #2295, and `merge_with_reuse.sh` runs `prepare_perf_changelog_merge.py canonicalize --pr-number ` unguarded (no `if !`), which calls `validate_added_pr_link` with a non-None `pr_number` — that strict path rejects any appended link that isn't the current PR's own link or an `XXX` placeholder. This is a genuine, reproducible break in the standard merge path even though CI's own validation invocation (no `--pr-number` flag) doesn't catch it.\n\n### Other factors\nThe fix is a one-line change (swap the hardcoded `pull/2231` for an `XXX` placeholder, which canonicalize auto-fills at merge time), but since it would actively break the merge tooling if left as-is, I'm deferring rather than approving. The bilingual-title nit is real per AGENTS.md and matches the sibling PRs #2231/#2232 pattern, but is metadata-only.
|
|
||
| - config-keys: | ||
| - dsv4-fp4-b200-vllm-agentic | ||
| scenario-type: | ||
| - agentic-coding | ||
| description: | ||
| - "Eval-only follow-up to PR #2231: restore the official DeepSeek-V4 vLLM tool-calling frontend flags --tool-call-parser deepseek_v4 and --enable-auto-tool-choice" | ||
| - "Re-run the dedicated AgentX SWE-bench eval to verify tool definitions are accepted instead of failing with 'tool parsing is disabled by frontend configuration'" | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2231 | ||
| evals-only: true |
There was a problem hiding this comment.
🔴 The newly appended perf-changelog.yaml entry sets pr-link to .../pull/2231 instead of this PR's own number (#2295). At merge time utils/merge_with_reuse.sh runs prepare_perf_changelog_merge.py canonicalize --pr-number 2295, which rejects any newly-appended entry whose pr-link isn't the current PR's own link or an XXX placeholder — so this will raise ChangelogValidationError and abort the standard merge tooling, even though it slips past CI. Fix: use an XXX placeholder for pr-link (auto-canonicalized at merge) and keep the #2231 reference in the description text only.
Extended reasoning...
The bug: The perf-changelog entry appended by this PR (lines 5006-5015) sets:
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2231but this PR is #2295, not #2231. utils/validate_perf_changelog.py enforces (via validate_added_pr_link, lines 144-160) that any newly appended entry's pr-link must equal https://github.com/SemiAnalysisAI/InferenceX/pull/<pr_number> for the PR that added it, or be one of the PR_LINK_PLACEHOLDERS ("XXX" / .../pull/XXX). Pointing a brand-new entry at a different, already-merged PR's number satisfies neither condition.
Why CI didn't catch it: The check-changelog step in run-sweep.yml invokes validate_perf_changelog.py with only --base-ref/--head-ref — no --pr-number flag exists on that script at all. With pr_number=None, validate_added_pr_link falls back to the looser branch (line 146-151) that just checks the link is some well-formed .../pull/<digits> URL via CANONICAL_PR_LINK.fullmatch. .../pull/2231 passes that regex trivially, so CI is green, and the PR description's own validation command (which mirrors exactly what CI runs) also doesn't exercise the stricter check.
Why merge would fail: utils/merge_with_reuse.sh unconditionally runs (around lines 181-186):
prepare_perf_changelog_merge.py canonicalize --base-ref origin/main --pr-number "$PR"
This calls canonicalize_appended_links() -> compare_entries(base, head, pr_number=2295) -> validate_added_pr_link(link, 2295). Now pr_number is not None, so the strict branch runs: expected = ".../pull/2295", and since .../pull/2231" is neither expected nor in PR_LINK_PLACEHOLDERS, it raises:
ChangelogValidationError: new PR entry must use '.../pull/2295' or an XXX placeholder; found '.../pull/2231'
prepare_perf_changelog_merge.py's main() catches this and returns 1, and this invocation in merge_with_reuse.sh isn't guarded with if ! (unlike the dry-run canonicalize call earlier in the script), so under set -euo pipefail the merge script aborts outright.
Step-by-step proof:
- Base
perf-changelog.yamlhas N entries; this PR's diff appends entry N+1 withpr-link: .../pull/2231. - Reviewer/CI runs
validate_perf_changelog.py --base-ref origin/main --head-ref HEAD(no--pr-number) →pr_number=None→ only regex-checks the link is well-formed → passes. PR merges "cleanly" from CI's perspective. - A maintainer later runs
utils/merge_with_reuse.sh 2295to actually integrate the PR. - That script calls
prepare_perf_changelog_merge.py canonicalize --pr-number 2295. compare_entrieswalks the appended entries, callsvalidate_added_pr_link(".../pull/2231", 2295).expected = ".../pull/2295";.../pull/2231"is not inPR_LINK_PLACEHOLDERSand !=expected→ChangelogValidationErrorraised →main()returns 1 →merge_with_reuse.shaborts underset -e.
The fix is a one-line change: set pr-link: XXX (or .../pull/XXX) instead of hardcoding .../pull/2231; the canonicalize step will substitute the real PR link (2295) automatically at merge time. The existing description text already references "#2231" for context, which is fine to keep — it's the structured pr-link field that must not point at a different PR.
| --no-enable-flashinfer-autotune | ||
| --tokenizer-mode deepseek_v4 | ||
| --reasoning-parser deepseek_v4 | ||
| --tool-call-parser deepseek_v4 | ||
| --enable-auto-tool-choice | ||
| --attention-config '{"backend":"FLASHINFER_MLA_SPARSE_DSV4","use_prefill_query_quantization":true,"use_fp4_indexer_cache":true}' | ||
| --no-disable-hybrid-kv-cache-manager | ||
| --disable-uvicorn-access-log |
There was a problem hiding this comment.
🟡 PR title and description are English-only, but AGENTS.md mandates bilingual PR titles/descriptions (format: <English title> / <中文标题> plus a Chinese section in the body).
Extended reasoning...
AGENTS.md explicitly states: "PR and GitHub-issue titles & descriptions must be bilingual — include a Simplified Chinese version in addition to English. Title format: <English title> / <中文标题>. In the PR/issue body, follow the English content with its Chinese translation (e.g. a ## 中文说明 section...). This applies to every PR and every issue."
PR #2295's title is "Restore DeepSeek-V4 tool calling for AgentX evals" — no / <中文标题> suffix. The body consists of ## What changed, ## Why, and ## Validation sections, all in English only, with no accompanying ## 中文说明 (or similar) Chinese section.
This is not a stylistic nitpick — it's a repository-specific, explicitly documented mandatory convention, and it's one this repo actively follows: recent merged commits on the same recipe lineage use the bilingual title format, e.g. 01e9e73 [LMCache] Add LMCache arm to official DSV4 FP4 B200 vLLM AgentX sweep / 将 LMCache 分支并入官方 DSV4 FP4 B200 vLLM AgentX 扫描 (#2231) and 9ecc56d [LMCache] Add LMCache arm on tuned DSV4 FP4 B300 vLLM AgentX recipe / 在调优后的 DSV4 FP4 B300 vLLM AgentX 配方上新增 LMCache 分支 (#2232). PR #2295 (this PR, a direct follow-up to #2231) breaks that pattern.
Step-by-step proof:
- Open AGENTS.md, line 7 — the bilingual title/body requirement is stated as mandatory ("must be bilingual"), with an explicit title format and a body-structure example.
- Compare against this PR's actual title:
Restore DeepSeek-V4 tool calling for AgentX evals. Applying the required format, it should read something likeRestore DeepSeek-V4 tool calling for AgentX evals / 恢复 DeepSeek-V4 AgentX 评测的工具调用能力. - Compare against this PR's body: sections are
## What changed,## Why,## Validation— no## 中文说明(or equivalent) section follows them, as the convention requires. - Cross-check against sibling/parent PRs in the same lineage ([LMCache] Add LMCache arm to official DSV4 FP4 B200 vLLM AgentX sweep / 将 LMCache 分支并入官方 DSV4 FP4 B200 vLLM AgentX 扫描 #2231, [LMCache] Add LMCache arm on tuned DSV4 FP4 B300 vLLM AgentX recipe / 在调优后的 DSV4 FP4 B300 vLLM AgentX 配方上新增 LMCache 分支 #2232) merged just before this one — both follow the bilingual format, confirming this isn't a dead-letter rule but one actively enforced in this exact recipe family.
Fix is trivial: retitle to Restore DeepSeek-V4 tool calling for AgentX evals / 恢复 DeepSeek-V4 AgentX 评测的工具调用能力 (or similar) and append a ## 中文说明 section summarizing the What changed / Why / Validation content in Chinese.
This has no runtime or functional impact — it's PR metadata, not code — so it doesn't block correctness, but it is a genuine, actionable deviation from a mandatory, explicitly documented repo convention that this PR's own lineage otherwise follows consistently.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29845648467 |
1 participant
What changed
--tool-call-parser deepseek_v4and--enable-auto-tool-choicein the DeepSeek-V4 B200 vLLM AgentX server command.Why
The #2231 SWE-bench eval received backend 500 responses because vLLM rejected requests containing tool definitions with
tool parsing is disabled by frontend configuration. These are the official DeepSeek-V4 vLLM frontend flags required to accept and parse tool calls.Validation
bash -n benchmarks/single_node/agentic/dsv4_fp4_b200_vllm.shpython3 utils/validate_perf_changelog.py --base-ref origin/main --head-ref HEAD --changelog-file perf-changelog.yamlagentic_evalsrow and no throughput rowspython3 -m pytest -q utils/test_process_changelog.py utils/matrix_logic/test_generate_sweep_configs.py(115 passed)Follow-up to #2231.